Conversation
Calling URLs via curl results in the error: “Command blocked by safety guard (path outside working dir)”. This commit fixes the problem.
yinwm
left a comment
yinwm
left a comment
There was a problem hiding this comment.
Code Review: Fix calling URLs with curl
Thanks for the PR! The fix direction is correct, but I found several issues that need to be addressed before merging.
🔴 Critical Issues
1. Incorrect Command Line Parsing
for _, part := range strings.Split(cmd, " ") {Splitting by space is incorrect for command-line parsing. It cannot handle quoted arguments:
# This command would be incorrectly split
curl "http://example.com/path with spaces"
# Result: ["curl", "\"http://example.com/path", "with", "spaces\""]Suggestion: Use shlex or a similar library for proper command-line argument parsing.
2. URL Detection Logic Has a Security Vulnerability
_, err := url.ParseRequestURI(strings.ReplaceAll(part, "\"", ""))
if err == nil {
continue
}url.ParseRequestURI accepts relative URIs, which means local paths like /etc/passwd would also parse successfully:
url.ParseRequestURI("/etc/passwd") // Returns nil error - treated as URL!This creates a security vulnerability: local paths could be misidentified as URLs and bypass the safety check.
Suggestion: Check if the URL has a scheme and host:
u, err := url.Parse(part)
if err == nil && u.Scheme != "" && u.Host != "" {
continue // Real URL, skip path check
}🟡 Medium Issues
3. Performance: Repeated Calculations Inside Loop
for _, part := range strings.Split(cmd, " ") {
// ...
cwdPath, err := filepath.Abs(cwd) // Called every iteration
// ...
pathPattern := regexp.MustCompile(`...`) // Compiled every iterationThese should be moved outside the loop.
4. Incomplete Quote Handling
strings.ReplaceAll(part, "\"", "")Only handles double quotes, not single quotes (') which are commonly used on Unix systems.
🟢 Suggested Refactor
The fix approach is correct, but the implementation needs improvement. Here's a suggested refactor:
if t.restrictToWorkspace {
// Check path traversal on entire command first
if strings.Contains(cmd, "..\\") || strings.Contains(cmd, "../") {
return "Command blocked by safety guard (path traversal detected)"
}
cwdPath, err := filepath.Abs(cwd)
if err != nil {
return ""
}
pathPattern := regexp.MustCompile(`[A-Za-z]:\\[^\\\"']+|/[^\s\"']+`)
matches := pathPattern.FindAllString(cmd, -1)
for _, raw := range matches {
// Check if it's a real URL (has scheme and host)
if u, err := url.Parse(raw); err == nil && u.Scheme != "" && u.Host != "" {
continue
}
p, err := filepath.Abs(raw)
if err != nil {
continue
}
rel, err := filepath.Rel(cwdPath, p)
if err != nil {
continue
}
if strings.HasPrefix(rel, "..") {
return "Command blocked by safety guard (path outside working dir)"
}
}
}Summary
| Category | Count |
|---|---|
| 🔴 Critical Issues | 2 |
| 🟡 Medium Issues | 2 |
| 🟢 Suggestions | 1 |
Verdict: The PR direction is correct, but the implementation has issues - particularly the URL detection logic which could lead to a security vulnerability. Please address these concerns before we can merge this.
fix Incorrect Command Line Parsing fix URL Detection Logic Has a Security Vulnerability fix Incomplete Quote Handling
|
@yinwm I fixed the problems from your review. |
3 participants
📝 Description
Calling URLs via curl results in the error: “Command blocked by safety guard (path outside working dir)”. This commit fixes the problem.
My changes to guardCommand ensure that the command is split into its individual parts (command + options/flags + arguments) and that each part is checked individually. In addition, web URLs are excluded from the check.
🗣️ Type of Change
🤖 AI Code Generation
☑️ Checklist